Exclude **/node_modules/** from .vscode/launch.json searches#1234
Exclude **/node_modules/** from .vscode/launch.json searches#1234testforstephen merged 2 commits intomicrosoft:mainfrom
Conversation
|
@gluxon this is a good finding, thanks for bringing it up for our awareness. |
| const results: vscode.Uri[] = await vscode.workspace.findFiles(".vscode/launch.json"); | ||
| // Excluding "**/node_modules/**" as a common cause of excessive CPU usage. | ||
| // https://github.com/microsoft/vscode/issues/75314#issuecomment-503195666 | ||
| const results: vscode.Uri[] = await vscode.workspace.findFiles(".vscode/launch.json", "**/node_modules/**"); |
There was a problem hiding this comment.
I'm OK with excluding both node_modules and .git.
There was a problem hiding this comment.
Thanks! So to be clear, updating this argument **/node_modules/** to {**/node_modules/**,.git/**} would be ideal?
I'll test that out to make sure I have the right glob syntax.
There was a problem hiding this comment.
It would be interesting if there is some performance comparison data to share.
There was a problem hiding this comment.
It looks like the 2nd argument appends rather than replaces. The existing command in this PR:
await vscode.workspace.findFiles(".vscode/launch.json", "**/node_modules/**");runs the following process:
'/Applications/Visual Studio Code.app/Contents/Resources/app/node_modules.asar.unpacked/@vscode/ripgrep/bin/rg' \
--files \
--hidden \
--case-sensitive \
-g '/.vscode/launch.json' \
-g '!**/node_modules/**' \
-g '!**/.git' \
-g '!**/.svn' \
-g '!**/.hg' \
-g '!**/CVS' \
-g '!**/.DS_Store' \
-g '!**/Thumbs.db' \
--no-ignore \
--follow \
--no-config \
The -g '!**/.git' exclusion comes from VS Code's default settings.
{
"files.exclude": {
"**/.git": true,
"**/.svn": true,
"**/.hg": true,
"**/CVS": true,
"**/.DS_Store": true,
"**/Thumbs.db": true
}
}It would be interesting if there is some performance comparison data to share.
Definitely. I was curious too, but hadn't had time until now to run some tests.
Without the **/node_modules/** exclusion, the rg process runs for over an hour at 1100%+ CPU in the repo my team works on. I can run the process overnight to see exactly how long it takes if that's something we're curious about.
Here's some data for comparing whether .git is excluded. For my specific machine and repo:
node_modules Excluded |
.git Excluded |
Trial 1 | Trial 2 | Average |
|---|---|---|---|---|
| ❌ | ✅ | 1 hour+ | - | 1 hour+ |
| ✅ | ❌ | 536ms | 493ms | 514.5ms |
| ✅ | ✅ | 788ms | 477ms | 632.5ms |
It looks like excluding .git is fairly negligible in terms of timing. Either way, I don't think we have to do something explicit here since .git should be excluded unless users explicitly set files.excludes["**/.git"] to false?
There was a problem hiding this comment.
Great to see the perf data, thanks for sharing that. Looks like it's enough to exclude "node_modules" only.
There was a problem hiding this comment.
Looks like this utility can get another improvement if we set the workspaceFolder as the base part of the relative pattern
And then limit the search maxResults to 1
There was a problem hiding this comment.
I created an issue to track the further optimization. #1252
There was a problem hiding this comment.
@jdneo Those both look like great suggestions. Thanks!
Thank you for reviewing and considering it! This change will be a big help to my team. |
|
@gluxon thanks for contribution. |
This is a strange PR and I would have no qualms with it being rejected.
Problem
Excluding
**/node_modules/**fromvscode.workspace.findFiles(...)calls is somewhat necessary to prevent excessive CPU usage when this extension is installed and users open a large frontend project in VS Code.findFilessearches the entire repo by default, even.gitignorefiles.My teammates are running into this excessive CPU usage problem as they jump between frontend repos and Java repos in VS Code.
Workaround
One comment from a VS Code member recommends extensions exclude
node_modulessince a proper fix to thefindFilesAPI would be a breaking change: microsoft/vscode#75314 (comment)This seems to be somewhat standard, even in extensions that have nothing to do with Node.js. For example:
workspace.findFilesin themicrosoft/vscoderepo shows almost every usage excluding**/node_modules/**explicitly. https://github.com/microsoft/vscode/search?q=workspace.findFilesI've also PR'ed a similar change in other repos:
Would this be reasonable in
vscode-java-debugas well?Risks
The docs on
workspace.findFiles(...)say an undefined 2nd argument defaults to the user'sfiles.excludesetting. Changing this argument to**/node_modules/**means this search no longer excludes the user configuredfiles.exclude. If that's a concern, I think we can concatenate**/node_modules/**after looking up thefiles.excludesetting.